Fix hub notes race condition: await refresh inline#8
Merged
drewburchfield merged 3 commits intomasterfrom Apr 3, 2026
Merged
Conversation
_ensure_fresh_counts() was using asyncio.create_task() to refresh connection counts in the background, causing get_hubs/get_orphans to query stale data on the first call. Now awaits the refresh inline so counts are always fresh before the query runs. Changes: - Await _refresh_all_counts() directly instead of create_task() - Wait for in-progress refresh instead of skipping when lock is held - Remove _handle_refresh_error callback (errors propagate via await) - Update tests to match new wait-instead-of-skip behavior
- Move staleness check inside _refresh_lock to prevent duplicate refreshes - Extract _do_refresh (no lock) from _refresh_all_counts (with lock) - Update CONCURRENCY.md to reflect wait-not-skip behavior - Rewrite race condition test to verify exactly 1 refresh with 20 callers - Fix hub_analyzer tests to match new atomic check+refresh pattern
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Fix test_staleness_check_skips_refresh_when_fresh to mock _do_refresh instead of _refresh_all_counts (vacuously true assertion) - Release pool connection before calling _do_refresh to avoid holding an idle connection during the entire refresh operation
drewburchfield
added a commit
that referenced
this pull request
Apr 3, 2026
…me migration - Revert connection_count back to = 0 on upsert (preserving counts breaks staleness detection which checks WHERE connection_count = 0; PR #8's inline-await already ensures fresh counts before queries) - Add DROP TRIGGER/FUNCTION to initialize() so existing databases get the migration on server start, not just from schema.sql
drewburchfield
added a commit
that referenced
this pull request
Apr 3, 2026
…ons (#10) * Fix code correctness issues (NAS-995) - Remove schema trigger that overwrites modified_at with CURRENT_TIMESTAMP, preserving the real file mtime passed by the indexer - Change hub_analyzer error type from ValueError to DatabaseError for consistency with the exception hierarchy - Fix validation docstrings: document that functions raise ValidationError instead of claiming they clamp values - Remove dead empty-content filter in indexer.py (already filtered at lines 119-122) - Remove unused _refresh_all_counts method from hub_analyzer and update test to call _do_refresh directly - Preserve connection_count on upsert (use notes.connection_count instead of resetting to 0) to avoid unnecessary full refreshes - Add ORDER BY chunk_index to graph_builder LIMIT 1 query for deterministic chunk selection - Fix weak test assertion: "pool" or "timeout" always evaluates True, changed to check membership in actual response text * Add migration: drop trigger for existing databases Existing databases retain the mtime-overwriting trigger even after the schema file is updated. Add DROP TRIGGER/FUNCTION IF EXISTS so the init script cleans up on restart. Also fix stale comment. * Address Devin review: revert connection_count preservation, add runtime migration - Revert connection_count back to = 0 on upsert (preserving counts breaks staleness detection which checks WHERE connection_count = 0; PR #8's inline-await already ensures fresh counts before queries) - Add DROP TRIGGER/FUNCTION to initialize() so existing databases get the migration on server start, not just from schema.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes NAS-990
Summary
_ensure_fresh_countsfrom fire-and-forget (asyncio.create_task) to inlineawait_refresh_lockto prevent TOCTOU race and duplicate refreshes_do_refresh()(no lock) from_refresh_all_counts()(with lock)_handle_refresh_errorcallback (errors propagate through await)Root cause
_ensure_fresh_counts()fired a background refresh and returned immediately. The query ran before the refresh completed, so first call toget_hub_notesalways returned empty results.Test plan